Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issues # 1689 and # 1697 #1734

Merged
merged 7 commits into from Jan 13, 2022
Merged

issues # 1689 and # 1697 #1734

merged 7 commits into from Jan 13, 2022

Conversation

anotherche
Copy link
Contributor

No description provided.

@@ -1393,7 +1408,7 @@ public synchronized Frame grabFrame(boolean doAudio, boolean doVideo, boolean do
AVRational time_base = video_st.time_base();
timestamp = 1000000L * pts * time_base.num() / time_base.den();
// best guess, AVCodecContext.frame_number = number of decoded frames...
frameNumber = (int)Math.round(timestamp * getFrameRate() / 1000000L);
frameNumber = (int)Math.floor(timestamp * getFrameRate() / 1000000L);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is floor() better than round()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While fixing #1697 I decided that in all cases when we round some floating results we should simulate result of integer division, so we should round towards zero, like floor does

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that when we use round(), we might be getting the next frame by mistake, but when we use floor() this doesn't happen, and in the worst case we might be getting the previous frame instead, which is better?

@@ -77,10 +77,15 @@

/** Constants defining data type in the frame*/
public static enum Type {
UNKNOWN,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make UNKNOWN = -1, making sure VIDEO = 0, and let's also follow the order of FFmpeg, unless there's a good reason not to: AVMEDIA_TYPE_UNKNOWN = -1, AVMEDIA_TYPE_VIDEO, AVMEDIA_TYPE_AUDIO, AVMEDIA_TYPE_DATA, AVMEDIA_TYPE_SUBTITLE, AVMEDIA_TYPE_ATTACHMENT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. I swapped data and subtitle by accident.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I forgot, but we can't set the ordinal() for enum, so for backward compatibility, we shouldn't put UNKNOWN first. Do we even need an UNKNOWN though? It might be problematic for forward compatibility. We could just initialize the field to null like all the rest of the fields. What do you think?

@anotherche
Copy link
Contributor Author

anotherche commented Jan 3, 2022 via email

@anotherche
Copy link
Contributor Author

anotherche commented Jan 3, 2022 via email

@saudet
Copy link
Member

saudet commented Jan 4, 2022

Ok, sounds good! Please update this branch accordingly. Thanks!

@anotherche
Copy link
Contributor Author

Updated the branch. In addition, I realized that my changes in grabFame contained another logical error: I used the pkt.stream_index(-1) technique in doData processing part to guarantee that a new packet is read on the next grabFrame call. However, because of this trick, this Data packet is not unreferenced, which will leak memory. To solve this problem, I abandoned setting the pkt.stream_index to -1, and changed the condition for reading the new package to the following explicit one.
boolean readPacket = (pkt.stream_index() != video_st.index() && pkt.stream_index() != audio_st.index());
I think this is correct, since we need to try to reuse packets related only to the selected audio and video streams, in all other cases we need to read a new packet.

@saudet
Copy link
Member

saudet commented Jan 5, 2022

We can set stream_index to -1 any time we want, we just need to make sure to call av_packet_unref() before doing that, such as here: https://github.com/bytedeco/javacv/blob/master/src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java#L714-L717
Could you try to do that instead?

@anotherche
Copy link
Contributor Author

But we just have set

// Export the stream byte data for non audio / video frames
frame.data = pkt.data().position(0).capacity(pkt.size()).asByteBuffer();
frame.opaque = pkt;

to return a frame with some data. Now if we call av_packet_unref() after it, what will be in the returned frame then?

@saudet
Copy link
Member

saudet commented Jan 5, 2022

Ah, yes, ok I see what you mean. So, readPacket is false on the first pass, but it gets set to true on the second pass here:

                if (!readPacket) {
                    readPacket = true;
                    continue;
                }

But you've removed that... why?

@anotherche
Copy link
Contributor Author

Well, because the logic including this part was initially incorrect.
First, if we get to the condition
} else if (doData) {
that means that all the previous if/else if were false, which means various variants, including when a packet (new or old) was from a stream different from the selected audio or video, OR it was from any stream not needed now (say, doAudio or doVideo is false). Then the doData handling part might return frame with data from ANY type of packet, not only data, subs or attachment.
So, I redesigned the condition for doData handling so that it to be explicit, so that it process only not audio and not video packets. In this new logic we need to read new packet if none of the specific if/else triggered - this is now done by the last else:
} else readPacket = true; //current packet is not needed (different stream index required)
And this is now the part which does the work corresponding to the previous code

                if (!readPacket) {
                    readPacket = true;
                    continue;
                }

So, now grabFrame correctly returns data packets if these packets are from data, subs or attachment streams only.
But at the next grabFrame call we must account for two things:

  1. we have to read new packet, because the previously read data packet may trigger the condition
    } else if (doData && pkt_stream_ind > -1 && streams.length > pkt_stream_ind && streams[pkt_stream_ind] != AVMEDIA_TYPE_VIDEO && streams[pkt_stream_ind] != AVMEDIA_TYPE_AUDIO) {
    again and again
  2. we have to unref previous data packet.

the fist task could be solved by setting stream index to -1 for that packet (as I suggested initially). But then it will not be unref. So we should retain its stream index as initial (this way the second task will be solved automatically if we set readPacket to true somhow - but how?). A possible way could be to check if frame.opaque is equal to the previous packet, and then to read new packet if it is true. But the frame.opaque may be changed in a user code (somewhere between two succeeding grabFrame calls) though. So I decided that the best way will be the change of the initial readPacket condition to the explicit form (we should try to reuse a previous packet for the selected audio and video streams only)

@anotherche
Copy link
Contributor Author

In general, this data processing problem occurs due to the fact that data is handled differently than audio or video. In the case of audio or video, we use the send_packet / receive_frame functions, which themselves suggest whether additional packets need to be read. In the case of data, we do not have such capabilities - we just have to return this data as frame.data and ensure that the new package is read on the next grabFrame call (and the old package is removed as well)

@saudet
Copy link
Member

saudet commented Jan 5, 2022

In general, this data processing problem occurs due to the fact that data is handled differently than audio or video. In the case of audio or video, we use the send_packet / receive_frame functions, which themselves suggest whether additional packets need to be read. In the case of data, we do not have such capabilities - we just have to return this data as frame.data and ensure that the new package is read on the next grabFrame call (and the old package is removed as well)

Right, the reason to use that block is precisely so that doData behaves like doVideo and doAudio. I suppose it doesn't work when doData is false, yeah. So maybe something like this?

            } else {
                if (!readPacket) {
                    readPacket = true;
                    continue;
                }
                if (doData ...
                }
            }

@anotherche
Copy link
Contributor Author

Yes, that should work correctly as well as the last suggested code in the framenumber branch. The only difference I can see is that processing data packets with

            } else {
                if (!readPacket) {
                    readPacket = true;
                    continue;
                }
                if (doData ...
                }
            }

will always require additional run of the external while(!done) loop to set readPacket=true, while in the case of

boolean readPacket = (pkt.stream_index() != video_st.index() && pkt.stream_index() != audio_st.index());

it is set true from the very beginning for any not video/audio packets. That can be slightly more efficient (but only in case of the data packets processing, which is not so often situation though). So now we have to determine which solution is the best.

@saudet
Copy link
Member

saudet commented Jan 5, 2022

Actually, something like looks more clean:

            } else if (readPacket && doData && whatever) {
                // ...
            } else if (!readPacket) {
                readPacket = true;
            }

That way, we don't have a continue out of no where, which makes it clear that readPacket is always going to be set and that we're eventually going to either process/return something, or reach the end of the file/stream.

@anotherche
Copy link
Contributor Author

anotherche commented Jan 6, 2022 via email

@saudet
Copy link
Member

saudet commented Jan 7, 2022

Ok. I agree. But why should we check readPacket in the last else?

We don't need to, we can just force it to true, sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants